-
Notifications
You must be signed in to change notification settings - Fork 9.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: introduce strict bash mode for scripts,.github #15519
Conversation
b3415cd
to
7837008
Compare
scripts/release_mod.sh
Outdated
@@ -10,7 +10,9 @@ | |||
# | |||
# % DRY_RUN=false REMOTE_REPO="origin" ./scripts/release_mod.sh push_mod_tags | |||
|
|||
set -e | |||
set -o pipefail |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to do it differently?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the late reply. Some scripts use this pattern, for instance https://github.com/etcd-io/etcd/blob/main/scripts/update_proto_annotations.sh#L6. I will change it into one line if the file doesn't run strict mode.
That's a lot of scripts changed, most of them are not tested on CI. Have you tested all of them manually? You don't need to migrate all scripts at once. We could split PR based on script type. Most important is |
Yes. I'm still testing it in my local. Will update it later.~ |
7837008
to
8963a57
Compare
@@ -120,7 +123,7 @@ function integration_pass { | |||
|
|||
function e2e_pass { | |||
# e2e tests are running pre-build binary. Settings like --race,-cover,-cpu does not have any impact. | |||
run_for_module "tests" go_test "./e2e/..." "keep_going" : -timeout="${TIMEOUT:-30m}" "${RUN_ARG[@]}" "$@" | |||
run_for_module "tests" go_test "./e2e/..." "keep_going" : -timeout="${TIMEOUT:-30m}" "${RUN_ARG[@]}" "$@" || return $? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
side note: fast fail return if there is any error from ./e2e/...
. If not, test.sh
just reports it fails but no error exit code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If not, test.sh just reports it fails but no error exit code.
Could you explain this? Isn't it a bug? The script should exit on error, because of set -e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The run_pass
is using if
to check the command result.
Lines 652 to 661 in 6b9c146
function run_pass { | |
local pass="${1}" | |
shift 1 | |
log_callout -e "\\n'${pass}' started at $(date)" | |
if "${pass}_pass" "$@" ; then | |
log_success "'${pass}' completed at $(date)" | |
else | |
log_error "FAIL: '${pass}' failed at $(date)" | |
exit 255 | |
fi |
Based on errexit
definition, https://www.gnu.org/software/bash/manual/bash.html#The-Set-Builtin
-e
Exit immediately if a pipeline (see Pipelines), which may consist of a single simple command (see Simple Commands), a list (see Lists of Commands), or a compound command (see Compound Commands) returns a non-zero status. The shell does not exit if the command that fails is part of the command list immediately following a while or until keyword, part of the test in an if statement, part of any command executed in a && or || list except the command following the final && or ||, any command in a pipeline but the last, or if the command’s return status is being inverted with !. If a compound command other than a subshell returns a non-zero status because a command failed while -e was being ignored, the shell does not exit. A trap on ERR, if set, is executed before the shell exits.
The shell does not exit if the command that fails is part of the command list immediately following a while or until keyword, part of the test in an if statement, part of any command executed in a && or || list except the command following the final && or ||, any command in a pipeline but the last, or if the command’s return status is being inverted with !.
For example,
➜ ~ cat /tmp/testing.sh
#!/usr/bin/env bash
set -euo pipefail
main() {
command -v xx # should fail
command -v ls
}
echo $SHELLOPTS
if ! main; then
echo oops
fi
main
➜ ~
➜ ~ /tmp/testing.sh
braceexpand:errexit:hashall:interactive-comments:nounset:pipefail
/usr/bin/ls
➜ ~
➜ ~ echo $?
1
➜ ~
So we should add || return
if the function has multiple calls 😂
EDIT: change
ctr
tols
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification.
The exit 255
at line 660 will still exit the script?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. exit 255
will exit the process(script)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really good catch!! It works after updating your example like below.
#!/usr/bin/env bash
set -euo pipefail
main() {
command -v xx || return $? # should fail
command -v ls
}
#echo $SHELLOPTS
if ! main; then
echo oops
fi
echo $?
main
@@ -170,8 +173,8 @@ function grpcproxy_e2e_pass { | |||
|
|||
# Builds artifacts used by tests/e2e in coverage mode. | |||
function build_cov_pass { | |||
run_for_module "server" run go test -tags cov -c -covermode=set -coverpkg="./..." -o "../bin/etcd_test" | |||
run_for_module "etcdctl" run go test -tags cov -c -covermode=set -coverpkg="./..." -o "../bin/etcdctl_test" | |||
run_for_module "server" run go test -tags cov -c -covermode=set -coverpkg="./..." -o "../bin/etcd_test" || return $? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
side note: fast fail return if there is any error during build
@@ -105,7 +105,7 @@ function relativePath { | |||
|
|||
#### Discovery of files/packages within a go module ##### | |||
|
|||
# go_srcs_in_module [package] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
side note: go_srcs_in_module doesn't receive any input.
ping @ahrtr @serathius please take a look. The fail one is tracked by #15545 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, what do you plan about TODOs left? Address in next PR? It's usually better to set TODOs on issues not specific people. This way they can be tracked.
REF: etcd-io#15514 Signed-off-by: Wei Fu <[email protected]>
8963a57
to
1fcb782
Compare
@serathius Updated with TODO and use #15549 to track it~ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
Thank you @fuweid
Thanks for the review. The TODO items will be handled in the follow-up. |
According to Github Actions [doc][1], linux platform will usebash -e {0}
mode to run script. But we also needpipefail
andnounset
in CI. This patch is aimed to add strict mode setting before any run.And remove the| tee
and egrep commands since we don't need this workaround to show error.[1]: https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idstepsshellREF: #15514
Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.